Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix detection of suitable architecture for conversion when LXD is clustered #14586

Merged
merged 1 commit into from
Dec 5, 2024
Merged

Fix detection of suitable architecture for conversion when LXD is clustered #14586

merged 1 commit into from
Dec 5, 2024

Conversation

MusicDin
Copy link
Member

@MusicDin MusicDin commented Dec 5, 2024 •
edited
Loading

Thanks to @escabo for reporting the issue where conversion fails if LXD is clustered and supports API extension instance_import_conversion.

Not sure how this slipped through for such a long time. Seems I have messed something up when juggling with commit order on initial implementation, as I remember configuring this.

Anyway, the issue is detection of suitable architecture, where source type was not matched with any case resulting in an error:

Error: Unknown instance source type "conversion"

tomponline
tomponline reviewed Dec 5, 2024
View reviewed changes
lxd/instance/instance_utils.go
@@ -547,7 +547,7 @@ func ResolveImage(ctx context.Context, tx *db.ClusterTx, projectName string, sou
// A nil list indicates that we can't tell at this stage, typically for private images.
func SuitableArchitectures(ctx context.Context, s *state.State, tx *db.ClusterTx, projectName string, sourceInst *cluster.Instance, sourceImageRef string, req api.InstancesPost) ([]int, error) {
// Handle cases where the architecture is already provided.
if shared.ValueInSlice(req.Source.Type, []string{"migration", "none"}) && req.Architecture != "" {
if shared.ValueInSlice(req.Source.Type, []string{"conversion", "migration", "none"}) && req.Architecture != "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a separate PR please can we have constants created for these values so we can see where they are referenced in the code.

@tomponline
Copy link
Member

@MusicDin @escabo why was architecture not populated btw?

@tomponline
Copy link
Member

@MusicDin once merged please can you prepare a cherry-pick against lxd-pkg-snap latest-candidate branch to include this commit in 6.2

@MusicDin
Copy link
Member Author

MusicDin commented Dec 5, 2024

Architecture was populated, but source.Type=conversion did not match any case in the function, reaching the end, where an error Error: Unknown instance source type "conversion" is returned.

@tomponline
Copy link
Member

Architecture was populated, but source.Type=conversion did not match any case in the function, reaching the end, where an error Error: Unknown instance source type "conversion" is returned.

why was clustering relevant?

@MusicDin
Copy link
Member Author

MusicDin commented Dec 5, 2024

why was clustering relevant?

When target server is not provided the architecture is used to decide upon suitable cluster member to run the new instance.

lxd/lxd/instances_post.go

Lines 1227 to 1259 in 6d29cd6

if s.ServerClustered && !clusterNotification && targetMemberInfo == nil {
architectures, err := instance.SuitableArchitectures(ctx, s, tx, targetProjectName, sourceInst, sourceImageRef, req)
if err != nil {
return err
}
// If no architectures have been ascertained from the source then use the default
// architecture from project or global config if available.
if len(architectures) < 1 {
defaultArch := targetProject.Config["images.default_architecture"]
if defaultArch == "" {
defaultArch = s.GlobalConfig.ImagesDefaultArchitecture()
}
if defaultArch != "" {
defaultArchID, err := osarch.ArchitectureId(defaultArch)
if err != nil {
return err
}
architectures = append(architectures, defaultArchID)
} else {
architectures = nil // Don't exclude candidate members based on architecture.
}
}
clusterGroupsAllowed := limits.GetRestrictedClusterGroups(targetProject)
candidateMembers, err = tx.GetCandidateMembers(ctx, allMembers, architectures, targetGroupName, clusterGroupsAllowed, s.GlobalConfig.OfflineThreshold())
if err != nil {
return err
}
}

@tomponline tomponline merged commit 9005251 into canonical:main Dec 5, 2024
25 checks passed
@MusicDin MusicDin deleted the fix/cluster-conversion branch December 5, 2024 09:33
@MusicDin MusicDin mentioned this pull request Dec 5, 2024
Merged
tomponline added a commit to canonical/lxd-pkg-snap that referenced this pull request Dec 5, 2024
@tomponline
lxd: Cherry-pick fix for conversion (latest-candidate) (#645)
afb00d0 
Cherry-pick
canonical/lxd@dd7b99e
(related PR: canonical/lxd#14586)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomponline tomponline tomponline approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MusicDin @tomponline